Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Introducing a common interface for counters. #762

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Ben-PH
Copy link

@Ben-PH Ben-PH commented May 22, 2024

Initially intended as a Clock rfc, my thoughts evolved while writing. Though this could be returned to a simple Clock RFC, a clock interface is nothing more than

  1. Reading a generic integer value that increments 1 at a time. For a clock, this is an oscilation count
  2. A specification of a measurement assosciated with each increment. For a clock, this is a duration of time

There are many situations where these two principals are used, and they are distinct only in the what they are reading (u32, u64, u8, etc), and what they are measuring (1/80_000_000 seconds per count? 1 milimeter per count? 100 milliradians per count?).

I chose to generalise, so as to entrench the idea that this RFC aims to bring in something that incurs no restrictions on reverse dependencies, and portability limitation defined only by the inherent nature of clocks and other counters.

I anticipate a Counter trait emerging, with other more specific expressions of counting to emerge, such as a Clock: Counter and PulseCount: Counter traits, in such a way that the extensions add additional constraints (e.g. you're counting something that can be a measurement of time). The RFC file goes into detail.

A list of reasonable expectations proposed by @jamesmunns

  • A timer/counter will need to be shared - there are relatively few of them on many chips
  • Users will want high precision in some cases, 1k-1M are very reasonable number
  • Some timers have a limited range - 16/24/32 bit are common, but many chips only have 16/24
  • Programs will be expected for months to years at a time
  • Some users want to use the lowest possible power design they can
  • Not all chips have atomics, and may need to use critical sections
  • Users may have other interrupts, some of which may have higher priority than your timer/counter interrupt
  • Users may want to use a global timer inside of other interrupts

@Ben-PH
Copy link
Author

Ben-PH commented May 22, 2024

meeting reference: #758 (comment)

Copy link
Member

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few notes, as somebody not on the HAL team, but present for many of the previous discussions.


```rust
// rotate brightness between 50 and 100% every 15 seconds using a clock-specific extension of a counting interface.
fn time_rotated_brightness<LedP: SetDutyCycle, ClkSrc: ReadClock>(led_pin: LedP, clk: ClkSrc) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this interface work well with shared access? e.g. &ClkSrc?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why that should be difficult, and if it was, why it should not be overcome.

One thing to consider, is the question of how this should differ from, say, an InputPin implementor. conceptually, it's no different. instead of reading an inherently boolean state, it's a count, with the addition of a specification of what's being counted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This typically becomes challenging if you need to have more complex inner state - e.g. expanding a 16-bit timer to larger ranges. Something to keep in mind if you need to change other parts of the design.

// rotate brightness between 50 and 100% every 15 seconds using a clock-specific extension of a counting interface.
fn time_rotated_brightness<LedP: SetDutyCycle, ClkSrc: ReadClock>(led_pin: LedP, clk: ClkSrc) {
let age: MiliSecs<_> = clk.read_count().time_scaled();
let age: u16 = age.into();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that this probably need to be fallible, for example if we are more than 66 seconds into the runtime.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. This specific code is useless outside of the scope of illustrating a point. Key ideas is to communicate a potential idiom.

This is the code defining the `embedded_time::Clock` trait. I have changed the comments for the context of this RFC:
```rust
// I would rename this to `TickCount`. A consistent oscilator is not necisarily a clock. It could be a PWM at 50% duty cycle,
// or an ab encoder tracking total distance moved, etc. The only assumption is that it is an incremental counter, and that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this address platforms where only down-counting timers exist? Are implementors expected to handle this (e.g. u32::MAX - raw_value?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point no. A good design would be mindful of behavior where countdown is a posibility, but for now, I'm thinking in torms of strictly incrementing counts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha - something to keep in mind. I have go figure out what platform it is, but IIRC a fairly typical platform only has down-counting timers available.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the assosciated type I'm going with at this point (might drop the Copy):

    /// Typically u32 or u64 (or held within a type-wrapper) that corresponds with the
    /// data type held within the peripheral register being read.
    type RawData: Copy;

I've been ruminating on this question a bit, and I can't find any technical, development, portability, etc. reason not to have this on the forefront. Right now, the only thing stopping me, is finding a chance to do a bit of a case-study of making a hal/driver/etc portable through this trait, and needs this behavior yo describe.

I anticipate it being (relatively) trivial. My guess is a hal impl would implement try_read_raw, and do the mapping you describe. They would set the raw-data type to be u32, do the u32::max - $REGISTER_READ (or similar), then provide their own mappings to a data type that encapsulates the thing they are measuring.

// each increment has a specific meaning (be it a meanure of time, distance, etc.).
pub trait Clock: Sized {

// This `T` assosciated type is constrained to u32 or u64. I agree with constraining to an unsigned integer, but I am of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If u32 is used, is there an opinion of what to do in the case of wraparounds? For example, 1MHz based timers are somewhat common and desired for timing, but this will wrap around a u32 in approximately 71 minutes. Even a 32.768kHz time will wrap a u32 in 1.5 days.

A u64 can probably practically be considered infinite, even at 1GHz, it would take ~584 years to wrap a u64.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be up to the implementor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"that would be up to the implementor" - but this affects downstream users (e.g. drivers consuming the trait). Can drivers rely that the number always goes up? If so, how is this behavior ensured if the implementor provides a 1MHz 32 bit timer that works for the first 71 minutes, but not afterwards?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nailing down this kind of three-way contract between the HAL implementor, the trait interface, and the trait-using driver is pretty much the whole challenge of stabilizing these APIs, there's only so much that can be left to the implementor, while still providing a useful contract to the consuming driver.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a draft of a concept where you can extend a time-counter:

/// There are many scenarios where a time-based tick-counter needs to handle the case
/// where the counter overflows. This trait is intended to provide a HAL with the means
/// of providing a `TimeCount` implementor with specialised interupt-based overflow
/// tracking. 
pub trait TimeWrap: TimeCount {
    /// A HAL implementation might register the provided method to handle an overflow
    /// interupt and increment a field that tracks overflow.
    ///
    /// META: I honestly don't know the best way to do this. This might prove to be
    ///       a key technical challenge
    fn register_wrap_interupt(self, ???);
}

Is it the good way? dunno. Keen to hear thoughts.

// Not exactly sure about how to read into this, or what should be done. Will update this part of RFC in response to
// questions, feedback, and any further insight I gather. My thinking is anything that respembles construction or
// initialisation is out, much the same as any other trait in embedded-hal.
fn new_timer<Dur: Duration>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this RFC propose a mechanism for multiplexing multiple timers, e.g. if new_timer is called multiple times? Is this Timer interface specified anywhere?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to think about this, but with respect to constructors, I anticipate not including it, much the same as with all the other traits in embedded-hal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's reasonable!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean "new_timer returns a new instance, but sometimes each instance just uses the same timer under the hood. What approach to allowing this is being proposed?", I see this as addressing the challenge of "shared state, not necisarily immutable", and using a multitude of multiplexed objects as one particular strategy.

I'm of the mind that the primary domain of portability sits within the the scope of how a thing is used. There could be a case for a "soft-standard" of construction, but the driving mandate should always be "portability in its use".

Some thoughts I've been reminating on:

  • Object exclusivity contradicts the premise of portability of count-readers. Time-based, or otherwise.
  • Shared mutable state is to be avoided, especially at the interface level.
  • Not being able to multiplex, or use other means to share a counter that's potentially mutable, is a deal-breaker.
  • Interior mutability patterns/idioms/lang-features/etc. at impl level should be able to resolve this contradiction nicely. Their use must not be limited in any way.
  • In particular, w.r.t. embedded, interior mutability must be available in the context of juggling interrupts, soft/hard-real-time use-cases, etc.

My gut is to use patterns of shared immutable state at the interface level, and ensure no restrictions are imposed to allow for sharing something that is imutable under the hood

To whit:

  • &self in the interface
  • Investigate the various multiplexing approaches used, and ensure that e-hal Just Works to make their solutions usable in a portable fashion.
  • Validate this in the context of embedded: interupts, real-time, etc.

If a particular implementation chooses to use multiple, unshared, mutable objects that multiplex a single thing under the hood, I don't see how that would be incompatible with the pattern I just proposed at first glance, but I expect to be proven wrong on that.

# Unresolved questions
[unresolved]: #unresolved-questions

How to move forward? I propose an `embedded-count` added to the rust-embedded repository, initially just a placeholder,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, it is generally required to first get a working system, usually with at least 2-3 platform implementations, EXTERNALLY first, so that they can be evaluated in working operation, prior to merging anything or creating new repos in the rust-embedded space.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, but I'm concerned about the chicken-egg paradox.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, however the HAL team is just as concerned about having/owning a crate that it isn't able to consider the design of.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that there is much context I do not account for in my original intent. I'll educate myself on the context/standards/expectations and update my declared thoughts on moving forward.

I'll fork it, and begin initial work. At an appropriate time, it will be upstreamed, and v0.0.2 will be released. If all goes
well, its form will be representative of this RFC.

Are we comfortable with using `typenum` instead of const generics. This will remove the limitations of const generics
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the underlying counter ticks are truly decoupled from the concept of time, it would be good to discuss how this would work in practice. Many platforms don't have hardware floating point support, and still many platforms (Cortex-M0, M0+, likely others) don't necessarily have hardware division support.

Will it be reasonable for these platforms to perform the tick->time conversions?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be up to each platform implementation. Fugit does everything using integers and in cost time with minor exceptions, and I wouldnt be surprised if platforms gravitated towards fugit primitives when counting for time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not saying this is a deal breaker - but something to actively address and engage with. If the base timer is 1MHz, it'll require integer division to render milliseconds or seconds.

It would be good to discuss, even if the answer is "fugit does it like this, it is this cheap, reasonably priced for the features provided".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way of interpreting this comment, is that it's a proposal to include a canonical expression of time when writing portable embedded. Either as part of this RFC, or in a seperate, but closely related one.

Here are my thoughts so far, from most, to least portable:

  • fugit: u32 or u64. extreme emphasis on zero runtime costs through the use of consts
  • embassy time: u64 only
  • core library is a u64 and u32, and doesn't use the concept of "ticks"

For me, the easiest answer to what is most portable is "whatever type you choose", but that's not much use, I think this is a void that needs filling.

My take is that at least the following properties are needed in order to be considered maximumly embedded-portable:

  • Is based on raw-data, such as tick-count (fugit, embassy-time)
  • Raw data is of any type. If it's not an integer type, it should be a thin-wrapper around an integer type (Foo(T), with T being any integer)
  • A compile-time encapsulation of scale. For time, this would be the tick-frequency. (fugit, embassy_time)
  • Zero-cost-abstraction aliasing between common representations, such as millis, micros, days, etc. (fugit, embassy-time)
  • A set of type-aliases that explicitly represent different time measures without cost, such as MilliSeconds<u32> (fugit)
  • implementation of common conversions meeting zero-cost-abstraction, such as Into<embassy_time::Duration/Instant>, Into<core::time::Duration>, should be trivially easy. A code-base that uses portable e-hal time, then calls into embedded-time, the part of the code that does the type-conversion should be optimised away in every-respect before the executable binary is generated.

I feel the closest is fugit, but for me to feel fugit as ready to integrate into e-hal, I percieve these as some of the blocking issues.

  • u32 and u64 only. that covers the vast majority of platforms, perhaps, but u8, 16, 128 exist as well, and for domains other than time, such as pulse-counters, you might want signed values. Multiple values are also a possible scenario (a tick-counter and a rollover counter, for example).
  • It uses const-generics for encapsulating scale (e.g. a 80hz u32 tick-count would be Instant<u32, 1u32, 80u32>), which means integrating with other interfaces is hit-or-miss (they might only be able to use usizes, or if they use u32, and you want to bridge the two, you're stuck with u32 types). It makes no sense to me that code must choose a machine-representation of a number to describe the frequency of an oscilator.
  • It's waiting on some unstable const-generic features to add useful features (e.g. to do changing between bases).
  • It's limited to time-only: If you want to count, for example, milliradians with a magnetic AB rotary encoder, that could work perfectly fine, but the naming would be Bizare.

I would be 100% behind setting up some sort of e-hal encapsulation of measuring time, and other things that counters... count, but there's some work involved to get there, imho.

# Alternatives
[alternatives]: #alternatives

embassy-time has an encapsulation of time, however that lives in an async-first context.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a note: embassy-time is fully usable in non-async contexts. There is no requirement to use async at all. There are certainly other drawbacks you could mention (requirements to use u64, separate externally defined driver methods, mandated timer wheel/queue implementation, etc.), but these are fully separate from "async or not".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should add that embassy time is not intended to be a universally applicable encapsulation of a counter, or something along those lines.

Are we comfortable with using `typenum` instead of const generics. This will remove the limitations of const generics
entirely, including the need for nightly features, at the risk of interface ergonomics, and compromising user familiarity.
I wish to explore the use of this crate, though I feel it's a core requirement that the change in UX to be trivial. I.e. other
than theneed to use `typenum::U5` where `5u32/64/size/8` would be used to set a generic const.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to fully demonstrate what the typenum-based interface (or any alternatives) would look like for users. See my top level notes wrt providing examples for discussion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate you writing this up! In particular, I would very much like to see the following items, which I don't see in the current draft:

  • discussion of potential wraparound conditions, particularly if u64 is not a mandatory representation of the counter value
  • discussion of how you expect platforms with limited range timers (e.g. 16-bit only timers) to participate for u32 and u64 based counter values
  • If necessary (particularly for increasing smaller timers to larger ranges), any external resources, such as interrupts, that may be necessary for implementation
  • How you expect sharing of these clock/counter resources to work, for example if multiple drivers need to use them to provide timing capabilities
  • Any additional API surface, such as how "wait for timer to complete" would look

I'd also really really like to see:

  • The concrete traits you are proposing, for example in a a published crate (on GitHub or crates-io)
  • At least one example of implementing these traits, for your hardware platform or HAL of choice, likely in a fork of an existing HAL
  • At least one example of a downstream driver crate, that uses this trait
  • At least one example of an application, that links the HAL-provided trait impl and the trait-consuming driver, e.g. an "end to end example"

These examples are vitally necessary for evaluation - both for you to determine if the proposed interface(s) are reasonable in practice, as well as for others to comprehend the full extent of what you are proposing.

I'm not on the HAL team, but I imagine all the items I have listed here (and in comments) will be strong points of discussion, so for your RFC to be accepted it should likely address or dismiss these concerns.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your first group of points illustrate the need to ensure that everything that is implementation detail be left to the implementor. If a hal author of a specific platform or usecase is in any way hampered in their ability for their implementation to be encapsulated appropriately, then that is a failure of the trait definitions. For example, platforms with 16 bit counters could track wrappings themselves, or they could just say "out counter wraps" or whatever they choose to do.

The proposal in this RFC is to standardise an interface, with the only restriction on implementation detail being at the type constraint level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, however, it is necessary to at least demonstrate that your proposed interface is practically usable before agreeing on that - and the best way we have today is by providing at least one (ideally a few diverse) implementations.

We've had many designs in embedded-hal that SEEMED like good ideas, or were good in isolation, but did not interact well beyond basic "hello world" interactions. This is particularly true in SPI and I2C, where it became clear between 0.2 and 1.0 that it was necessary to address sharing of a bus, as many practical systems had more than one device on the bus.

@jamesmunns
Copy link
Member

CC @rust-embedded/hal team for review - I'm not sure if this is exactly how we decide on HAL traits at the moment (or if this should be in the /embedded-hal repo), but this seems a reasonable place to start the discussion, and certainly is a contentious enough topic historically to potentially shake out in RFC form.

@David-OConnor
Copy link

David-OConnor commented May 22, 2024

What I think would be useful: Something similar to std::time::Instant's functionality. How I currently handle it, using a HAL hardware timer method:

let timestamp = tick_timer.get_timestamp_ms();
if timestamp - self.last_channel_change > HOP_INTERVAL {
    self.last_channel_change = timestamp;
    // Do things.
}

In ISR, to handle wrapping:

#[interrupt]
/// Increments the tick overflow.
fn TIM3() {
    timer::clear_update_interrupt(3);
    TICK_OVERFLOW_COUNT.fetch_add(1, Ordering::Relaxed);
}

@Ben-PH
Copy link
Author

Ben-PH commented May 22, 2024

What I think would be useful: Something similar to std::time::Instant's functionality. How I currently handle it, using a HAL hardware timer method:

let timestamp = tick_timer.get_timestamp_ms();
if timestamp - self.last_channel_change > HOP_INTERVAL {
    self.last_channel_change = timestamp;
    // Do things.
}

In ISR, to handle wrapping:

#[interrupt]
/// Increments the tick overflow.
fn TIM3() {
    with(|cs| {
        access_global!(TICK_TIMER, timer, cs);
        timer.clear_interrupt(TimerInterrupt::Update);
  });
    TICK_OVERFLOW_COUNT.fetch_add(1, Ordering::Relaxed);
}

I agree that the question of overflow is... conspicuous. I've made a mental note. Don't know where I'll go with it, but my initial gut is to have a way of registering interupt handlers as part of the interface. somithngi like

fn register_overflow_interupt(&mut self, ???)

to lean into your example, you would pass in TIM3, or an equivilent closure, and the implementation detail when it comes to doing the read would use some sort of code that is in the first half.

That's going to be a headache and a half, but would definately need to be addressed at some point.

@David-OConnor
Copy link

Yea - I guess the issue is here, overflow is likely going to happen depending on the underlying time keeper. It will have to be managed. In the example I posted, this is handled using an incrementing atomic wrap counter that both the firmware (to update), and time-getting-code (to use).

The API to get and use the timestamps (Instants etc) shouldn't expose wraps though.

@David-OConnor
Copy link

David-OConnor commented May 22, 2024

Inspired by this thread, I've updated the usage code (in my code bases) to this; uses an Instant that's similar to std::Instant, and implements addition/subtraction with core::Duration:

let timestamp = tick_timer.elapsed(); // `Instant` type.
if (timestamp - self.last_channel_change).as_millis() > HOP_INTERVAL {
    self.last_channel_change = timestamp;
    // Do things
}

@Ben-PH
Copy link
Author

Ben-PH commented May 22, 2024

The base of my thoughts is that when you think "counter", especially in an embedded context, the notion of it wrapping is well within scope. Just not sure about what shape it should take at the interface level.

@Ben-PH
Copy link
Author

Ben-PH commented May 23, 2024

@David-OConnor I've updated the document with an initial attempt to address this. Would be interested in your thoughts:

https://github.com/rust-embedded/wg/pull/762/files#diff-66a0e54eb567d4eac263c2c1fca7467e332d8c8346d98da53ae554159ea97f67R175-R194

@David-OConnor
Copy link

I'm not great with trait design, but I think that should work? Overall: The timer struct in question should be able to A: Store a value that counts wraps. B: Be able to increment (or reset?) that counter.

Of note, an alternative is to, instead of specifying it in the trait, is to use a public atomic defined in the struct's module. The struct uses that to calculate elapsed time, and it is updated without access to the struct in a timer interrupt etc. That way ownership, critical sections etc isn't required to handle the wrap.

@Ben-PH
Copy link
Author

Ben-PH commented May 23, 2024

@David-OConnor I'm on a bit of a mission to fork-and-demo. Basically, I have this repo going: https://github.com/Ben-PH/counter-proposal/ and want to see how it fits together with as many hals/drivers/embedded projects as possible. I've already got this put together: Ben-PH/esp-hal-downstream@ef763f4 and doing that was quite informative. I would be glad to see what it looks like in the context of your work.

@jannic
Copy link
Member

jannic commented May 28, 2024

Dirbaio pointed out on matrix that rust-embedded/embedded-hal#357 contains important information about past design considerations that are very much relevant for this RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants